-
Notifications
You must be signed in to change notification settings - Fork 67
[GEN][ZH] Fix buffer overrun and memory leaks in listbox properties of GUIEdit #796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GEN][ZH] Fix buffer overrun and memory leaks in listbox properties of GUIEdit #796
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Also it looks like there are a ton of memory leaks in this code. Perhaps can tackle that in a follow up change. Some of these new char
can also be changed to stack buffers.
Yeah, I noticed that as well. I'll try to make some time to open a new pr for the memory leaks. Would you say that this would be the right approach? (Char vs. char and sizeof vs. 60)
EDIT: On second thought, it's probably preferable to put those memory leak fixes in this pr instead of a separate one. |
Yes this looks good.
You can do that yes. |
I think the memory leaks are handled now. |
GeneralsMD/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this change? Does it work?
GeneralsMD/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
Outdated
Show resolved
Hide resolved
I haven't been able to test, so I'm afraid I can't answer that. |
It would would be good to give this a brief test before commit. |
I have increased the percentages buffer sizes to 200 to hold a string like "1,1,1,1 ...." (100 * '1' + 100 * ','). From my brief testing the issues that this pr addresses appear to be fixed. I did notice, however, that creating a new horizontal slider immediately crashes because of a nullptr dereference in getImageWidth(). |
GeneralsMD/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Fixes a potential 'heap' overflow in GUIEdit: the actual buffer size is smaller than the claimed buffer size passed to GetDlgItemTextA.